[육선우] sprint6#125
Hidden character warning
Conversation
…ser and Channel api
…dd UserAuthApi and ReadStatusApi
joonfluence
left a comment
joonfluence
left a comment
There was a problem hiding this comment.
전체 요약
Discord 클론 앱(Discodeit)의 Spring Boot + JPA 백엔드와 React + TypeScript 프론트엔드를 구현한 PR입니다. 전반적으로 잘 구조화되어 있으나, 평문 비밀번호 비교, N+1 쿼리, 불필요 파일 커밋 등 보안 및 성능 이슈가 존재합니다.
라인 코멘트로 달 수 없는 항목
[P3] .gitignore - 불필요 파일 커밋
.DS_Store, .discodeit/*.ser, data/storage/*, .postman/config.json, out/production/ 등 바이너리/시리얼라이즈 파일, macOS 시스템 파일, 로컬 스토리지 데이터, 빌드 결과물이 리포지토리에 포함되어 있습니다. .gitignore에 추가하고 git rm --cached로 제거해 주세요.
[P4] channelStore.ts - 공격적인 폴링 주기
채널 목록을 3초마다 폴링하는 것은 상당히 공격적입니다. 채널 목록은 자주 변경되지 않으므로 10-30초 간격이면 충분합니다. 장기적으로는 WebSocket 도입을 권장합니다.
| .orElseThrow( | ||
| () -> new NoSuchElementException("User with username " + username + " not found")); | ||
|
|
||
| if (!user.getPassword().equals(password)) { |
There was a problem hiding this comment.
[P1] 🔒 평문 비밀번호 비교 — 보안 취약점
비밀번호를 평문으로 비교하고 있습니다. 회원가입 시에도 평문 저장(User 생성자에서 그대로 this.password = password), 로그인 시에도 equals()로 비교합니다.
반드시 BCrypt 등 해시 알고리즘을 사용해야 합니다.
| if (!user.getPassword().equals(password)) { | |
| if (!passwordEncoder.matches(password, user.getPassword())) { |
회원가입(BasicUserService.create)에서도 passwordEncoder.encode()로 해싱 후 저장해야 합니다.
| spring: | ||
| datasource: | ||
| url: jdbc:postgresql://localhost:5432/discodeit | ||
| username: discodeit_user |
There was a problem hiding this comment.
[P1] 🔒 DB 자격 증명 하드코딩
DB 자격 증명이 소스코드에 하드코딩되어 있습니다. 환경 변수나 Spring Profiles, Vault 등을 통해 외부화하세요.
| username: discodeit_user | |
| username: ${DB_USERNAME} | |
| password: ${DB_PASSWORD} |
| @Transactional | ||
| public ChannelDto update(UUID channelId, PublicChannelUpdateRequest request) { | ||
| Channel channel = channelRepository.findById(channelId) | ||
| .orElseThrow(() -> new NoSuchElementException("Channel not found")); |
There was a problem hiding this comment.
[P2] 🐛 userId 파라미터 무시 — 보안 이슈
findAllByUserId(UUID userId) 메서드인데 userId 파라미터를 완전히 무시하고 channelRepository.findAll()로 모든 채널을 반환합니다. Private 채널까지 모든 사용자에게 노출되는 보안 이슈입니다.
userId에 해당하는 채널만 필터링하는 로직이 필요합니다:
- PUBLIC 채널 전체 + 해당 userId의 ReadStatus가 있는 PRIVATE 채널만 반환
| public ChannelDto toDto(Channel entity) { | ||
| if (entity == null) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
[P2] ⚡ 심각한 N+1 쿼리 문제
채널 목록 조회 시 채널마다:
messageRepository.findAllByChannelId()— 모든 메시지를 조회해 마지막 메시지 시간 계산readStatusRepository.findAllByChannelId()— 모든 ReadStatus 조회userRepository.findById()— 참여자별 개별 User 조회
채널 수가 늘면 성능이 급격히 저하됩니다.
개선안:
lastMessageAt:findTopByChannelIdOrderByCreatedAtDesc()같은 단일 쿼리 사용- participants:
@EntityGraph나 JOIN FETCH로 한 번에 조회 - Mapper에서 Repository를 직접 호출하는 것 자체가 안티패턴입니다. Service 레이어에서 필요한 데이터를 미리 조회하여 Mapper에 전달하는 구조를 권장합니다.
|
|
||
|
|
||
| @Mapping(source = "profile", target = "profile") | ||
| @Mapping(target = "online", constant = "false") |
There was a problem hiding this comment.
[P2] 🐛 온라인 상태 항상 false로 하드코딩
UserStatus.isOnline() 로직이 구현되어 있지만, Mapper에서 constant = "false"로 하드코딩하여 모든 사용자가 항상 오프라인으로 표시됩니다.
MapStruct의 @Mapping(expression = ...)이나 커스텀 매핑 메서드를 사용하여 실제 UserStatus.isOnline() 결과를 반영해야 합니다.
| public class Channel extends BaseEntity { | ||
|
|
||
| @Id | ||
| @GeneratedValue(strategy = GenerationType.UUID) |
There was a problem hiding this comment.
[P2] 🐛 BaseEntity와 중복된 @id 선언 + 타입 불일치
BaseEntity를 상속하면서@Id UUID id를 재선언하여 중복 매핑 문제가 발생합니다.BaseEntity에 이미@Id가 정의되어 있으므로 제거하세요.createdAt은LocalDateTime,updatedAt은Instant로 타입이 불일치합니다. 일관되게Instant를 사용하세요.@Setter가 Entity 클래스 레벨에 붙어 있어 캡슐화가 깨집니다. 필요한 필드만 setter를 제공하거나 도메인 메서드로 변경하세요.
ReadStatus 엔티티에서도 동일한 id/createdAt/updatedAt 재선언 문제가 있습니다.
|
|
||
|
|
||
| @ManyToOne(fetch = FetchType.LAZY) | ||
| @JoinColumn(name = "user_id", insertable = false, updatable = false) |
There was a problem hiding this comment.
[P2] 같은 컬럼에 대한 이중 매핑
user_id 컬럼을 @ManyToOne User user (insertable=false, updatable=false)와 @Column UUID userId 두 필드에 매핑하고 있습니다. 데이터 불일치 위험이 있고 JPA 설계로서 혼란스럽습니다.
User 관계만 유지하고 userId는 user.getId()로 접근하는 것이 바람직합니다. 또는 반대로 userId만 유지하고 필요 시 조회하는 방식을 선택하세요.
|
|
||
| @ExceptionHandler(IllegalArgumentException.class) | ||
| public ResponseEntity<String> handleException(IllegalArgumentException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
[P3] e.printStackTrace() 대신 Logger 사용 필요
e.printStackTrace()는 stdout으로 출력되어 로그 수집 시스템에서 누락됩니다. SLF4J Logger를 사용하세요.
| e.printStackTrace(); | |
| log.warn("Bad request: {}", e.getMessage(), e); |
또한 ErrorResponse DTO가 정의되어 있지만 사용하지 않고 plain String을 반환하고 있습니다. 일관된 에러 응답 형식을 위해 ErrorResponse를 활용하세요.
| try { | ||
| this.updateLastActiveAt(userId); | ||
| } catch (Exception e) { | ||
| } |
There was a problem hiding this comment.
[P3] 조회 메서드의 부수효과(side effect) + 빈 catch 블록
find() (단순 조회)에서 updateLastActiveAt()을 호출하는 것은 예측하기 어려운 부수효과입니다. 또한 빈 catch 블록으로 모든 예외를 삼키고 있어 디버깅이 불가능합니다.
lastActiveAt 갱신은 별도 API 엔드포인트나 이벤트로 분리하세요.
| open-in-view: false | ||
| database-platform: org.hibernate.dialect.PostgreSQLDialect | ||
| hibernate: | ||
| ddl-auto: update |
There was a problem hiding this comment.
[P4] ddl-auto: update는 프로덕션에서 위험
ddl-auto: update는 프로덕션에서 예기치 않은 스키마 변경을 유발할 수 있습니다. 개발 환경 전용임을 명시하거나, 프로필별로 분리하세요. Flyway/Liquibase 같은 마이그레이션 도구 도입도 고려해 보세요.
2 participants
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게